Skip to content

Conversation

@aeubanks
Copy link
Contributor

Otherwise the range doesn't make sense since we interpret it as signed.

Fixes #134115

Otherwise the range doesn't make sense since we interpret it as signed.

Fixes llvm#134115
@llvmbot
Copy link
Member

llvmbot commented Apr 23, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Arthur Eubanks (aeubanks)

Changes

Otherwise the range doesn't make sense since we interpret it as signed.

Fixes #134115


Full diff: https://github.com/llvm/llvm-project/pull/137053.diff

2 Files Affected:

  • (modified) llvm/lib/Transforms/IPO/FunctionAttrs.cpp (+6-2)
  • (modified) llvm/test/Transforms/FunctionAttrs/initializes.ll (+14)
diff --git a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
index bbfed2ac2c090..5af68df6f4463 100644
--- a/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ b/llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -661,8 +661,12 @@ ArgumentAccessInfo getArgumentAccessInfo(const Instruction *I,
     auto TypeSize = DL.getTypeStoreSize(Ty);
     if (!TypeSize.isScalable() && Offset) {
       int64_t Size = TypeSize.getFixedValue();
-      return ConstantRange(APInt(64, *Offset, true),
-                           APInt(64, *Offset + Size, true));
+      APInt Low(64, *Offset, true);
+      APInt High(64, *Offset + Size, true);
+      // Bail if the range overflows signed 64-bit int.
+      if (Low.sge(High))
+        return std::nullopt;
+      return ConstantRange(Low, High);
     }
     return std::nullopt;
   };
diff --git a/llvm/test/Transforms/FunctionAttrs/initializes.ll b/llvm/test/Transforms/FunctionAttrs/initializes.ll
index 861c61d683ae0..937595b5e9b74 100644
--- a/llvm/test/Transforms/FunctionAttrs/initializes.ll
+++ b/llvm/test/Transforms/FunctionAttrs/initializes.ll
@@ -635,3 +635,17 @@ define void @memset_offset_1_size_0(ptr %dst, ptr %src) {
   call void @llvm.memmove.p0.p0.i64(ptr %dst.1, ptr %src, i64 0, i1 false)
   ret void
 }
+
+; We should bail if the range overflows a singed 64-bit int.
+define void @range_overflows_signed_64_bit_int(ptr %arg) {
+; CHECK: Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(argmem: write)
+; CHECK-LABEL: define void @range_overflows_signed_64_bit_int(
+; CHECK-SAME: ptr writeonly captures(none) [[ARG:%.*]]) #[[ATTR0]] {
+; CHECK-NEXT:    [[GETELEMENTPTR:%.*]] = getelementptr i8, ptr [[ARG]], i64 9223372036854775804
+; CHECK-NEXT:    store i32 0, ptr [[GETELEMENTPTR]], align 4
+; CHECK-NEXT:    ret void
+;
+  %getelementptr = getelementptr i8, ptr %arg, i64 9223372036854775804
+  store i32 0, ptr %getelementptr
+  ret void
+}

// Bail if the range overflows signed 64-bit int.
if (Low.sge(High))
return std::nullopt;
return ConstantRange(Low, High);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check isSignWrappedSet() on the result instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see this, thanks for pointing it out. Although I ended up just using APInt::sadd_ov

return ConstantRange(APInt(64, *Offset, true),
APInt(64, *Offset + Size, true));
APInt Low(64, *Offset, true);
APInt High(64, *Offset + Size, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will probably still make ubsan unhappy for your test case?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aeubanks aeubanks merged commit 0547e84 into llvm:main Apr 23, 2025
9 of 10 checks passed
@aeubanks aeubanks deleted the initializes branch April 23, 2025 22:56
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…llvm#137053)

Otherwise the range doesn't make sense since we interpret it as signed.

Fixes llvm#134115
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FunctionAttrs: Assertion `isOrder edRanges(RangesRef)' failed.

3 participants